feat(cli): stash wizard thin-wrapper subcommand#394
Conversation
🦋 Changeset detectedLatest commit: e999857 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughThe PR adds a new ChangesWizard Subcommand Implementation & Integration
Sequence DiagramsequenceDiagram
participant CLI as CLI User
participant Stash as stash wizard<br/>Command
participant PM as Package Manager<br/>Detector
participant FS as File System<br/>(cache check)
participant Spawn as Node spawn()
participant Wizard as `@cipherstash/wizard`<br/>Process
CLI->>Stash: stash wizard [args...]
Stash->>PM: Detect project PM<br/>(npm/pnpm/yarn/bun)
PM-->>Stash: npm | pnpm | yarn | bun
Stash->>Stash: Build runner cmd<br/>(npx | pnpm dlx | yarn dlx | bunx)
Stash->>FS: Check if wizard cached
alt First Run (not cached)
FS-->>Stash: Not found
Stash->>CLI: Print "downloading (~5s)..."
else Warm Cache
FS-->>Stash: Found
Stash->>CLI: Print "Launching wizard..."
end
Stash->>Spawn: spawn(bin, [preArgs, '@cipherstash/wizard', ...args],<br/>{ stdio: 'inherit' })
Spawn->>Wizard: Start with inherited TTY
Wizard->>Wizard: User interaction
Wizard->>Spawn: Exit with code
Spawn-->>Stash: close event + exit code
Stash->>CLI: process.exit(code)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/cli/src/commands/db/install.ts (1)
271-288: ⚡ Quick win
printNextStepshardcodesstash wizardinstead of using the pm-aware runner.All three init providers compute the wizard step as
${runnerCommand(pm, '@cipherstash/cli')} wizard, producing e.g.pnpm dlx@cipherstash/cliwizardfor pnpm users.printNextStepsignores the package manager entirely and emits a barestash wizard, which is wrong for any user who randb installvia a one-shot runner (no globalstashbinary on PATH).Both
detectPackageManagerandrunnerCommandare already imported at line 5, so the fix is minimal:♻️ Proposed fix
-function printNextSteps(): void { +function printNextSteps(pm = detectPackageManager()): void { + const cli = runnerCommand(pm, '@cipherstash/cli') p.note( [ 'Next steps:', '', ' 1. Wire up encrypt/decrypt with the wizard (AI-guided, automated):', - ' stash wizard', + ` ${cli} wizard`, '',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/db/install.ts` around lines 271 - 288, printNextSteps currently hardcodes the string "stash wizard" instead of using the package-manager-aware runner; update printNextSteps to call detectPackageManager() and compute the one-shot runner via runnerCommand(pm, '@cipherstash/cli'), then use `${runner} wizard` (or equivalent) in the message instead of the hardcoded "stash wizard" so the displayed next-step works for pnpm/npm/yarn one-shot invocations; refer to the existing functions detectPackageManager and runnerCommand and the printNextSteps function to locate where to change the message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/bin/stash.ts`:
- Around line 274-281: Add an E2E test that exercises the new `wizard` routing
in `stash.ts` by spawning the CLI and asserting exit codes: call the binary with
arguments ["wizard","--help"] and assert it exits with code 0, and call a
failing/unknown wizard invocation (e.g., ["wizard","--invalid-flag"] or a wizard
mode that you know returns non-zero) and assert the process exit code is the
expected non-zero value; use Node's child_process.spawn/execFile in a Jest/Mocha
test to capture exit events and stdout/stderr, and reference the `wizardCommand`
behavior so the test ensures exit-code propagation through the `case 'wizard'`
branch.
In `@packages/cli/src/commands/wizard/index.ts`:
- Around line 60-67: The Windows failure is caused by using node's
child_process.spawn (spawn) with shell: false which cannot execute package
manager .cmd shims (npm/pnpm/yarn); replace the use of spawn in this block with
cross-spawn's default export (import spawn from 'cross-spawn') and call it with
the same signature (spawn(bin, args, { stdio: 'inherit' })) so .cmd is appended
on Windows; keep the child.on('close', ...) and child.on('error', ...) handlers
and the exitCode Promise logic unchanged and ensure TypeScript types/imports are
updated to reference cross-spawn instead of child_process.spawn.
---
Nitpick comments:
In `@packages/cli/src/commands/db/install.ts`:
- Around line 271-288: printNextSteps currently hardcodes the string "stash
wizard" instead of using the package-manager-aware runner; update printNextSteps
to call detectPackageManager() and compute the one-shot runner via
runnerCommand(pm, '@cipherstash/cli'), then use `${runner} wizard` (or
equivalent) in the message instead of the hardcoded "stash wizard" so the
displayed next-step works for pnpm/npm/yarn one-shot invocations; refer to the
existing functions detectPackageManager and runnerCommand and the printNextSteps
function to locate where to change the message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2fb4021c-5b21-4d68-9ff1-8efa18f0fc80
📒 Files selected for processing (12)
.changeset/cli-stash-wizard-subcommand.mdpackages/cli/src/bin/stash.tspackages/cli/src/commands/db/install.tspackages/cli/src/commands/index.tspackages/cli/src/commands/init/providers/__tests__/base.test.tspackages/cli/src/commands/init/providers/__tests__/drizzle.test.tspackages/cli/src/commands/init/providers/__tests__/supabase.test.tspackages/cli/src/commands/init/providers/base.tspackages/cli/src/commands/init/providers/drizzle.tspackages/cli/src/commands/init/providers/supabase.tspackages/cli/src/commands/wizard/__tests__/index.test.tspackages/cli/src/commands/wizard/index.ts
| case 'wizard': { | ||
| // Forward everything after `stash wizard` verbatim. The wizard package | ||
| // owns its own flag parsing; we don't try to interpret its surface | ||
| // here so it can evolve independently. | ||
| const wizardArgs = process.argv.slice(3) | ||
| await wizardCommand(wizardArgs) | ||
| break | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
E2E test required by coding guidelines for this routing change.
The wizard case introduces a new exit-code propagation path (wizardCommand calls process.exit(exitCode)) routed through stash.ts. The coding guidelines explicitly require an E2E test when touching src/bin/stash.ts "argv parsing, exit codes, or top-level error handling". The PR's test plan covers only unit tests for splitRunner; no E2E test for stash wizard routing is listed.
At minimum, an E2E test should verify that stash wizard --help (or equivalent) exits 0 and that an unknown/failing wizard invocation propagates the expected non-zero exit code.
As per coding guidelines: "Add an E2E test when touching src/bin/stash.ts argv parsing, exit codes, or top-level error handling."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/bin/stash.ts` around lines 274 - 281, Add an E2E test that
exercises the new `wizard` routing in `stash.ts` by spawning the CLI and
asserting exit codes: call the binary with arguments ["wizard","--help"] and
assert it exits with code 0, and call a failing/unknown wizard invocation (e.g.,
["wizard","--invalid-flag"] or a wizard mode that you know returns non-zero) and
assert the process exit code is the expected non-zero value; use Node's
child_process.spawn/execFile in a Jest/Mocha test to capture exit events and
stdout/stderr, and reference the `wizardCommand` behavior so the test ensures
exit-code propagation through the `case 'wizard'` branch.
| const exitCode = await new Promise<number>((resolvePromise) => { | ||
| const child = spawn(bin, args, { stdio: 'inherit', shell: false }) | ||
| child.on('close', (code) => resolvePromise(code ?? 0)) | ||
| child.on('error', (err) => { | ||
| p.log.error(`Failed to launch wizard: ${err.message}`) | ||
| resolvePromise(127) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check package.json engines field and any CI matrix for Windows runners
rg -n '"engines"' packages/cli/package.json
fd -e yml -e yaml . .github/workflows --exec grep -l 'windows'Repository: cipherstash/stack
Length of output: 78
🏁 Script executed:
cat packages/cli/package.json | head -100Repository: cipherstash/stack
Length of output: 1741
🏁 Script executed:
rg -A 5 '"engines"' packages/cli/package.jsonRepository: cipherstash/stack
Length of output: 97
🏁 Script executed:
cat -n packages/cli/src/commands/wizard/index.ts | sed -n '50,75p'Repository: cipherstash/stack
Length of output: 925
🏁 Script executed:
rg -B 5 -A 15 'function splitRunner|export.*splitRunner|const splitRunner' packages/cli/src/commands/wizard/Repository: cipherstash/stack
Length of output: 2181
🏁 Script executed:
rg -n "splitRunner" packages/cli/src/commands/wizard/index.tsRepository: cipherstash/stack
Length of output: 212
🏁 Script executed:
rg -B 10 "const runner = " packages/cli/src/commands/wizard/index.tsRepository: cipherstash/stack
Length of output: 622
🏁 Script executed:
rg -B 20 "function splitRunner" packages/cli/src/commands/wizard/index.ts | head -40Repository: cipherstash/stack
Length of output: 790
🏁 Script executed:
rg -A 30 "function runnerCommand|export.*runnerCommand" packages/cli/src/commands/init/utils.tsRepository: cipherstash/stack
Length of output: 745
🏁 Script executed:
rg -A 20 "function detectPackageManager|export.*detectPackageManager" packages/cli/src/commands/init/utils.tsRepository: cipherstash/stack
Length of output: 734
🏁 Script executed:
rg -n "windows|Windows|WIN32|win32" packages/cli/README.md packages/cli/CHANGELOG.md 2>/dev/nullRepository: cipherstash/stack
Length of output: 43
🏁 Script executed:
find packages/cli -name "*.md" -type f | xargs grep -l "Windows\|windows\|platform" 2>/dev/nullRepository: cipherstash/stack
Length of output: 45
🏁 Script executed:
rg "spawn.*bin.*shell.*false" packages/cli/src/ -A 5 -B 5Repository: cipherstash/stack
Length of output: 944
spawn with shell: false will fail on Windows for npm, pnpm, and yarn.
These package managers are distributed as .cmd batch scripts on Windows, and spawn with shell: false cannot execute them directly — the same ENOENT behavior is confirmed in the Node.js ecosystem. Only bunx (a native .exe) works as-is.
While Windows support is not explicitly declared in the engines field, the code will fail on Windows regardless. The standard fix in the Node.js ecosystem is to replace spawn with cross-spawn, which transparently appends .cmd on Windows:
🐛 Proposed fix using cross-spawn
-import { spawn } from 'node:child_process'
+import spawn from 'cross-spawn'cross-spawn is a drop-in replacement with identical call signature and is already a common dependency in CLI tooling. Alternatively, if adding a new dependency is undesirable, a minimal inline shim works:
- const child = spawn(bin, args, { stdio: 'inherit', shell: false })
+ const winBin = process.platform === 'win32' ? `${bin}.cmd` : bin
+ const child = spawn(winBin, args, { stdio: 'inherit', shell: false })Note: shell: true is not a safe workaround because passthroughArgs are user-supplied and would be subject to shell injection.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const exitCode = await new Promise<number>((resolvePromise) => { | |
| const child = spawn(bin, args, { stdio: 'inherit', shell: false }) | |
| child.on('close', (code) => resolvePromise(code ?? 0)) | |
| child.on('error', (err) => { | |
| p.log.error(`Failed to launch wizard: ${err.message}`) | |
| resolvePromise(127) | |
| }) | |
| }) | |
| const exitCode = await new Promise<number>((resolvePromise) => { | |
| const winBin = process.platform === 'win32' ? `${bin}.cmd` : bin | |
| const child = spawn(winBin, args, { stdio: 'inherit', shell: false }) | |
| child.on('close', (code) => resolvePromise(code ?? 0)) | |
| child.on('error', (err) => { | |
| p.log.error(`Failed to launch wizard: ${err.message}`) | |
| resolvePromise(127) | |
| }) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/wizard/index.ts` around lines 60 - 67, The Windows
failure is caused by using node's child_process.spawn (spawn) with shell: false
which cannot execute package manager .cmd shims (npm/pnpm/yarn); replace the use
of spawn in this block with cross-spawn's default export (import spawn from
'cross-spawn') and call it with the same signature (spawn(bin, args, { stdio:
'inherit' })) so .cmd is appended on Windows; keep the child.on('close', ...)
and child.on('error', ...) handlers and the exitCode Promise logic unchanged and
ensure TypeScript types/imports are updated to reference cross-spawn instead of
child_process.spawn.
The wizard ships as @cipherstash/wizard so the agent SDK stays out of the CLI bundle. Re-add a `stash wizard` subcommand that spawns the wizard via the project's package manager (npx / pnpm dlx / yarn dlx / bunx) so users only have to think about one CLI surface. Flags after `wizard` are forwarded verbatim. Cold-cache runs print an explicit "first run downloads ~5s" line so the CLI doesn't appear hung while the package manager resolves the package; warm-cache runs print a single "Launching..." line and hand the terminal over to the wizard. Existing copy that pointed at `npx @cipherstash/wizard` (init's next-steps for base/Drizzle/Supabase, db install's post-install note, the help banner) now uses `stash wizard`.
coderdan
force-pushed
the
dan/stash-wizard-wrapper
branch
from
4ab8e16 to
ce70b4d
Compare
The wrapper changes the runner-aware string the providers emit from `<runner> @cipherstash/wizard` to `<runner> stash wizard`. The provider unit tests were updated; the e2e suite checked the wizard hint via a `steps.find` and was missed. Updates the expectation so the find lands.
1 participant
Summary
Re-adds
stash wizardas a thin spawn wrapper around@cipherstash/wizard. The wizard ships as a separate npm package (the agent SDK is too heavy to bundle intostash); this wrapper exposes it as astashsubcommand so users only have to think about one CLI surface.npx,pnpm dlx,yarn dlx, orbunx— withstdio: 'inherit'.wizardverbatim (sostash wizard --debugworks).npx @cipherstash/wizard(init's next-steps for base / Drizzle / Supabase,db install's post-install note, the help banner) tostash wizard.This is the prerequisite PR for #395 — the new
initflow's "Use the CipherStash Agent" handoff option calls into the same wrapper.Why a wrapper instead of folding the wizard back in
CJ split the wizard out in #368 (May 1) precisely to keep the
stashCLI bundle lean. Pulling the agent SDK back in would undo that. The wrapper is onespawncall — the wizard package is still optional and only resolved at run-time.Test plan
pnpm --filter stash test(123 tests, including 6 new for runner-string splitting)pnpm --filter stash buildcleanpnpm --filter @cipherstash/e2e test:e2e(21 tests including the runner-string assertion that initially caught a missed copy update — now fixed)node packages/cli/dist/bin/stash.js wizardin a fresh project — cold-cache message + spawn confirmedstash --helpshows thewizardline + exampleFiles changed
packages/cli/src/commands/wizard/index.ts— newpackages/cli/src/bin/stash.ts— case + help bannerpackages/cli/src/commands/index.ts— exportpackages/cli/src/commands/init/providers/{base,drizzle,supabase}.ts— replacenpx @cipherstash/wizardreference withstash wizardpackages/cli/src/commands/db/install.ts— same replacement in the post-install notepackages/cli/src/commands/init/providers/__tests__/*.test.ts— assertion updatese2e/tests/package-managers.e2e.test.ts— assertion update.changeset/cli-stash-wizard-subcommand.md— minor bump